New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

remove HTTPClientResponse destructor #273

Closed
wants to merge 1 commit into
base: master
from

Conversation

Projects
None yet
3 participants
@carlor
Contributor

carlor commented Jul 28, 2013

It allocates memory, regardless of if the user has correctly called dropBody(); being a destructor, this throws InvalidMemoryOperationException.

Here's a debugger backtrace:

Breakpoint 1, 0x000000010022ebd8 in onInvalidMemoryOperationError ()
(gdb) bt
#0  0x000000010022ebd8 in onInvalidMemoryOperationError ()
#1  0x00000001002365c2 in D2gc3gcx2GC12mallocNoSyncMFmkPmZPv ()
#2  0x000000010023653a in D2gc3gcx2GC6mallocMFmkPmZPv ()
#3  0x0000000100235b20 in gc_malloc ()
#4  0x000000010022edcc in D4core6memory2GC6mallocFNaNbmkZPv ()
#5  0x00000001000fb602 in D4vibe5utils6memory11GCAllocator5allocMOFmZAv () at /Users/nathanmswan/.dvm/compilers/dmd-2.063.2/bin/../src/druntime/import/core/memory.d:335
#6  0x00000001000e4cb7 in D4vibe5utils5array22__T13AllocAppenderTAhZ13AllocAppender7reserveMFmZv () at ../vibe.d/source/vibe/utils/array.d:67
#7  0x00000001000e4914 in D4vibe6stream6memory18MemoryOutputStream7reserveMFmZv () at ../vibe.d/source/vibe/stream/memory.d:45
#8  0x00000001000e5d43 in D4vibe6stream10operations9readUntilFC4vibe4core6stream11InputStreamxAhmOC4vibe5utils6memory9AllocatorZAh () at ../vibe.d/source/vibe/stream/operations.d:79
#9  0x00000001000e5cc0 in D4vibe6stream10operations8readLineFC4vibe4core6stream11InputStreammAyaOC4vibe5utils6memory9AllocatorZAh () at ../vibe.d/source/vibe/stream/operations.d:36
#10 0x00000001000bd2f8 in D4vibe4http6common18ChunkedInputStream9readChunkMFZv () at ../vibe.d/source/vibe/stream/operations.d:35
#11 0x00000001000bce9d in D4vibe4http6common18ChunkedInputStream6__ctorMFC4vibe4core6stream11InputStreamZC4vibe4http6common18ChunkedInputStream () at ../vibe.d/source/vibe/http/common.d:329
#12 0x000000010004d93b in D3std4conv77__T7emplaceTC4vibe4http6common18ChunkedInputStreamTC4vibe4core6stream6StreamZ7emplaceFAvKC4vibe4core6stream6StreamZC4vibe4http6common18ChunkedInputStream () at /Users/nathanmswan/.dvm/compilers/dmd-2.063.2/bin/../src/phobos/std/conv.d:3767
#13 0x00000001000fdd99 in D4vibe5utils6memory56__T11FreeListRefTC4vibe4http6common18ChunkedInputStreamZ11FreeListRef37__T6opCallTC4vibe4core6stream6StreamZ6opCallFC4vibe4core6stream6StreamZS4vibe5utils6memory56__T11FreeListRefTC4vibe4http6common18ChunkedInputStreamZ11FreeListRef () at ../vibe.d/source/vibe/utils/memory.d:490
#14 0x00000001000ba3b4 in D4vibe4http6client18HTTPClientResponse10bodyReaderMFNdZC4vibe4core6stream11InputStream () at ../vibe.d/source/vibe/http/client.d:458
#15 0x00000001000ba96a in D4vibe4http6client18HTTPClientResponse8dropBodyMFZv () at ../vibe.d/source/vibe/http/client.d:517
#16 0x00000001000ba1e9 in D4vibe4http6client18HTTPClientResponse6__dtorMFZv () at ../vibe.d/source/vibe/http/client.d:440
#17 0x00000001000bc305 in D4vibe4http6client18HTTPClientResponse10__aggrDtorMFZv () at ../vibe.d/source/vibe/http/client.d:436
#18 0x000000010023ef6b in rt_finalize2 ()
#19 0x0000000100239c26 in D2gc3gcx3Gcx11fullcollectMFZm ()
#20 0x0000000100238e8e in D2gc3gcx3Gcx8bigAllocMFmPPS2gc3gcx4PoolPmZPv ()
#21 0x000000010023670c in D2gc3gcx2GC12mallocNoSyncMFmkPmZPv ()
#22 0x000000010023653a in D2gc3gcx2GC6mallocMFmkPmZPv ()
#23 0x0000000100235b82 in gc_qalloc ()
@mihails-strasuns

This comment has been minimized.

Show comment
Hide comment
@mihails-strasuns

mihails-strasuns Jul 28, 2013

Contributor

@nogc , I demand you! :)

P.S. Any GC operations are illegal in destructors, if there is one, worth checking others too.

Contributor

mihails-strasuns commented Jul 28, 2013

@nogc , I demand you! :)

P.S. Any GC operations are illegal in destructors, if there is one, worth checking others too.

@s-ludwig

This comment has been minimized.

Show comment
Hide comment
@s-ludwig

s-ludwig Jul 30, 2013

Member

The problem is that I didn't come up with a proper solution for this. Just removing the finalizer will possibly leave the connection in a bad state and can cause corrupted protocol talk or, in the best case, a timeout error on the other side of the connection. That's why I wanted to at least have an assertion. But since assert() also allocates when it triggers, it also doesn't help. Maybe using asm int 10; would work for x86, but then GDC and other architectures need to be handled separately.

That GC really needs to get properly fixed. It's just too easy to accidentially trigger something GC related (possibly indirectly because a class contains a reference counted field) and this restriction completely precludes a lot of useful or important use cases.

Member

s-ludwig commented Jul 30, 2013

The problem is that I didn't come up with a proper solution for this. Just removing the finalizer will possibly leave the connection in a bad state and can cause corrupted protocol talk or, in the best case, a timeout error on the other side of the connection. That's why I wanted to at least have an assertion. But since assert() also allocates when it triggers, it also doesn't help. Maybe using asm int 10; would work for x86, but then GDC and other architectures need to be handled separately.

That GC really needs to get properly fixed. It's just too easy to accidentially trigger something GC related (possibly indirectly because a class contains a reference counted field) and this restriction completely precludes a lot of useful or important use cases.

@mihails-strasuns

This comment has been minimized.

Show comment
Hide comment
@mihails-strasuns

mihails-strasuns Jul 30, 2013

Contributor

@s-ludwig You can throw pre-allocated exception (simply stored in immutable __gshared variable). AFAIK it is defined behaviour in D (destructor is not guaranteed to run per http://dlang.org/class.html#destructors)

Contributor

mihails-strasuns commented Jul 30, 2013

@s-ludwig You can throw pre-allocated exception (simply stored in immutable __gshared variable). AFAIK it is defined behaviour in D (destructor is not guaranteed to run per http://dlang.org/class.html#destructors)

@s-ludwig

This comment has been minimized.

Show comment
Hide comment
@s-ludwig

s-ludwig Jul 30, 2013

Member

Good idea. I'll do that.

Member

s-ludwig commented Jul 30, 2013

Good idea. I'll do that.

s-ludwig added a commit that referenced this pull request Jul 30, 2013

@s-ludwig

This comment has been minimized.

Show comment
Hide comment
@s-ludwig

s-ludwig Jul 30, 2013

Member

@carlor: Thanks for initiating this! I'll close the ticket.

Member

s-ludwig commented Jul 30, 2013

@carlor: Thanks for initiating this! I'll close the ticket.

@s-ludwig s-ludwig closed this Jul 30, 2013

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment